Make ConverterManager.getInstance() init thread-safe.
#776
+10
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We encountered a TSAN error because 2 threads were racing to call `ConverterManager.getInstance()`. My read is that the current code could actually be unsafe, since one thread might see `INSTANCE` as non-null before the other thread has initialized its fields. Fortunately, this looks like a good case for [the initialization-on-demand holder idiom](https://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#:~:text=initialization%20on%20demand%20holder%20idiom). (There would be additional thread-safety concerns if anyone were to try to _mutate_ the `ConverterManager` instance concurrently with usage. But I'm not sure there's a solution to that short of spraying `synchronized` around. That seems like probably overkill, given the possible performance impact for what I'd hope is an uncommon use case.)
|
Looks like a good change and happy to merge, but unfortunately |
|
Sorry, I did not anticipate reflection in the tests :) I'll take care of it and then actually run tests, which apparently I had neglected to do. |
|
The tests now pass. I updated the assertion to look for the field in the new class. I now check that that class is But I'm happy to update it in some other way if you think that would be best. |
jodastephen
approved these changes
Apr 4, 2024
|
Thanks! |
1 task
github-merge-queue bot
referenced
this pull request
in camunda/camunda
Sep 16, 2024
This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [joda-time:joda-time](https://www.joda.org/joda-time/) ([source](https://redirect.github.com/JodaOrg/joda-time)) | `2.12.7` -> `2.13.0` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>JodaOrg/joda-time (joda-time:joda-time)</summary> ### [`v2.13.0`](https://redirect.github.com/JodaOrg/joda-time/releases/tag/v2.13.0) [Compare Source](https://redirect.github.com/JodaOrg/joda-time/compare/v2.12.7...v2.13.0) See the [change notes](https://www.joda.org/joda-time/changes-report.html#a2.13.0) for more information. #### What's Changed - Make `ConverterManager.getInstance()` init thread-safe. by [@​cpovirk](https://redirect.github.com/cpovirk) in [https://github.com/JodaOrg/joda-time/pull/776](https://redirect.github.com/JodaOrg/joda-time/pull/776) - Add website page about secutity/CVEs by [@​jodastephen](https://redirect.github.com/jodastephen) in [https://github.com/JodaOrg/joda-time/pull/781](https://redirect.github.com/JodaOrg/joda-time/pull/781) - fix: include native-image files correctly by [@​klopfdreh](https://redirect.github.com/klopfdreh) in [https://github.com/JodaOrg/joda-time/pull/784](https://redirect.github.com/JodaOrg/joda-time/pull/784) - Enhance TZDB compiler to better match spec by [@​jodastephen](https://redirect.github.com/jodastephen) in [https://github.com/JodaOrg/joda-time/pull/786](https://redirect.github.com/JodaOrg/joda-time/pull/786) - Update GitHub actions to latest versions by [@​jodastephen](https://redirect.github.com/jodastephen) in [https://github.com/JodaOrg/joda-time/pull/788](https://redirect.github.com/JodaOrg/joda-time/pull/788) - Fix TZDB compiler %z parsing by [@​jodastephen](https://redirect.github.com/jodastephen) in [https://github.com/JodaOrg/joda-time/pull/787](https://redirect.github.com/JodaOrg/joda-time/pull/787) - Update tzdb handling by [@​jodastephen](https://redirect.github.com/jodastephen) in [https://github.com/JodaOrg/joda-time/pull/789](https://redirect.github.com/JodaOrg/joda-time/pull/789) - Update time zone data to 2024bgtz by [@​github-actions](https://redirect.github.com/github-actions) in [https://github.com/JodaOrg/joda-time/pull/790](https://redirect.github.com/JodaOrg/joda-time/pull/790) #### New Contributors - [@​cpovirk](https://redirect.github.com/cpovirk) made their first contribution in [https://github.com/JodaOrg/joda-time/pull/776](https://redirect.github.com/JodaOrg/joda-time/pull/776) - [@​klopfdreh](https://redirect.github.com/klopfdreh) made their first contribution in [https://github.com/JodaOrg/joda-time/pull/784](https://redirect.github.com/JodaOrg/joda-time/pull/784) **Full Changelog**: JodaOrg/joda-time@v2.12.7...v2.13.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/camunda/camunda). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC43NC4xIiwidXBkYXRlZEluVmVyIjoiMzguODAuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiYXV0b21lcmdlIl19-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We encountered a TSAN error because 2 threads were racing to call
ConverterManager.getInstance(). My read is that the current code couldactually be unsafe, since one thread might see
INSTANCEas non-nullbefore the other thread has initialized its fields.
Fortunately, this looks like a good case for the
initialization-on-demand holder
idiom.
(There would be additional thread-safety concerns if anyone were to try
to mutate the
ConverterManagerinstance concurrently with usage. ButI'm not sure there's a solution to that short of spraying
synchronizedaround. That seems like probably overkill, given the possible
performance impact for what I'd hope is an uncommon use case.)